Repository navigation
feat: add prepare image layout math - #57
Conversation
📝 WalkthroughWalkthroughThis PR introduces two domain modules to support the prepare-image feature. The output path module resolves requested image paths to Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domain/output_path.ts`:
- Around line 16-26: The current existence-check-then-return loop (using
existsSync on normalizedPath and candidate) has a TOCTOU race condition; replace
it with an atomic-create approach: inside the loop that constructs candidate
names (using parse, join and parsed.name), attempt to atomically create the file
(e.g., open/create with exclusive flag like fs.open/ openSync with 'wx' or
create a temp file and fs.rename) and only return the candidate after the atomic
create succeeds; on EEXIST (file already created by another process) continue
the loop and ensure any opened file descriptor is closed if used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b30ff6f4-c3f1-408c-b52d-a95bdff93379
📒 Files selected for processing (5)
docs/plans/v1-prepare-image-layout.mdsrc/domain/output_path.tssrc/domain/prepare_image_layout.tstests/output_path.test.tstests/prepare_image_layout.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
**/*.{js,ts,jsx,tsx}: Prioritize using plain JavaScript/TypeScript instead of libraries for fundamental algorithms (e.g., use Array methods instead of lodash)
Use functional programming patterns and immutable data structures in JavaScript/TypeScript code
Files:
tests/output_path.test.tssrc/domain/output_path.tssrc/domain/prepare_image_layout.tstests/prepare_image_layout.test.ts
tests/**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use explicit 30 second timeout for ffmpeg-heavy tests instead of relying on Bun's default 5 second per-test timeout
Files:
tests/output_path.test.tstests/prepare_image_layout.test.ts
src/domain/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Place deterministic policy, analysis, export, report, benchmark, and validation logic in
src/domain
Files:
src/domain/output_path.tssrc/domain/prepare_image_layout.ts
🔇 Additional comments (21)
src/domain/output_path.ts (2)
29-36: LGTM!
1-2: LGTM!tests/output_path.test.ts (4)
8-14: LGTM!
16-21: LGTM!
23-27: LGTM!
29-35: LGTM!src/domain/prepare_image_layout.ts (8)
1-52: LGTM!
54-102: LGTM!
104-127: LGTM!
129-152: LGTM!
154-180: LGTM!
182-188: LGTM!
190-194: LGTM!
196-200: LGTM!docs/plans/v1-prepare-image-layout.md (1)
1-19: LGTM!tests/prepare_image_layout.test.ts (6)
5-25: LGTM!
27-45: LGTM!
47-64: LGTM!
66-79: LGTM!
81-97: LGTM!
99-114: LGTM!
|
@coderabbitai review\n\nAll active review threads have been addressed or replied to and resolved. Please re-review the current stack state. |
|
✅ Actions performedFull review triggered. |
b7c5e58 to
b3f3c25
Compare
5c28c6b to
1ae3a8c
Compare
1ae3a8c to
b00efbf
Compare
Dismissed after addressed review thread was resolved and fresh CodeRabbit status passed.
Merge activity
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b00efbf1b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (extension === "") { | ||
| return `${requestedPath}.jpg`; | ||
| } |
There was a problem hiding this comment.
When requestedPath ends with a path separator and the directory does not exist yet (for example --out exports/), this branch treats it as an extensionless file path and returns exports/.jpg instead of rejecting it as a directory target. That creates a hidden file and violates the CLI contract that --out must be a file path; users who pass a directory path by mistake will get surprising output placement rather than a clear error.
Useful? React with 👍 / 👎.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Note
Add image layout math and output path resolution for prepare-image
computePrepareImageLayoutinsrc/domain/prepare_image_layout.tswhich selects a target canvas (landscape 3:2 up to 2160×1440, portrait 3:4 up to 1440×1920), scales borders, and returns render dimensions, offsets, and an optional centered cover crop.resolvePrepareImageOutputPathinsrc/domain/output_path.tswhich normalizes the output extension to.jpg, creates parent directories, and suffixes with-<index>to avoid overwriting existing files.Macroscope summarized b00efbf.